refactor: currency code validation and helpers - #4757
Merged
Conversation
Contributor
📝 WalkthroughWalkthrough
ChangesCurrency code validation
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
chrisgacsal
added
the
release-note/ignore
chrisgacsal
marked this pull request as ready for review
July 21, 2026 08:35
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/currencyx/currency.go (1)
289-294: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse an
elsebranch for better readability.For cleaner control flow and to mirror the
FiatCurrency.Validate()structure just above it, consider combining these consecutiveifchecks into anif-elseblock.♻️ Proposed refactor
if c.def == nil { errs = append(errs, errors.New("currency is not initialized")) - } - - if c.def != nil { + } else { if err := validateCustomCurrencyCode(Code(c.def.ISOCode)); err != nil { errs = append(errs, err) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/currencyx/currency.go` around lines 289 - 294, Update the validation logic in the currency validation method around c.def so the existing c.def != nil check becomes the else branch of the c.def == nil condition. Preserve both existing behaviors: report the uninitialized-currency error when c.def is nil, otherwise validate the custom currency code.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pkg/currencyx/currency.go`:
- Around line 289-294: Update the validation logic in the currency validation
method around c.def so the existing c.def != nil check becomes the else branch
of the c.def == nil condition. Preserve both existing behaviors: report the
uninitialized-currency error when c.def is nil, otherwise validate the custom
currency code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2fec9e5f-fc42-4416-90f6-b016e898a59c
📒 Files selected for processing (3)
pkg/currencyx/code.gopkg/currencyx/code_test.gopkg/currencyx/currency.go
currencyx.Code now accepts valid custom currency codes, but ledger accounting remains fiat-only because amount precision and booking semantics depend on fiat definitions. Centralize the boundary in ledger.ValidateCurrency and reuse it from customer balance validation so custom codes fail early with ErrCurrencyInvalid while malformed codes retain their existing validation behavior. Add coverage for valid fiat, invalid, and unsupported custom currency codes.
chrisgacsal
added
release-note/misc
turip
approved these changes
Jul 21, 2026
chrisgacsal
enabled auto-merge (squash)
July 21, 2026 09:26
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
currencyx.Codeto represent and validate both fiat and custom currency codesmodels.Equaler[Code]with exact, case-sensitive equalityCode.Type()to classify three-character fiat codes and custom currency codesImpact
Callers can use
currencyx.Codeconsistently for fiat and custom currencies, compare codes through the standardmodels.Equalercontract, and determine the currency type without constructing a full currency value.Validation
go vet ./pkg/currencyx/...go test ./pkg/currencyx/...Summary by CodeRabbit
|, enforces length bounds, and blocks fiat-recognized values).Greptile Summary
This PR centralizes currency-code behavior and keeps ledger accounting limited to fiat currencies. The main changes are:
Confidence Score: 5/5
This looks safe to merge.
Important Files Changed
Reviews (3): Last reviewed commit: "fix: reject custom currencies in ledger ..." | Re-trigger Greptile
Context used (3)